Swap out Safe requirements for generic multisig - #32
Conversation
|
Note this is tested with multisigs but NOT Safes - however based on my previous experience of implementing multisig Root Key Holders this pattern should work. |
|
Can we just have no check and let the FIP guide it? One of the (other) problems with the multisig check is that it makes testing really annoying. Being able to spin this up in a devnet for an integration test, or in butterflynet, or other places where you need lightweight exercise of the various paths here is that you also need to bring along all of the mechanisms needed to satisfy this check just to make stuff happen. Hard-mode is having to deploy Safe and jumping through all those hoops, I'm not sure this version is all that much easier? |
| /// @notice The only thing we require of a multisig: it can list its signers. | ||
| /// @dev Safe{Wallet} and the other common multisig implementations all expose this. | ||
| interface IMultisig { | ||
| function getOwners() external view returns (address[] memory); |
There was a problem hiding this comment.
Does Safe implement this method? Can we assume any multisig would implement it? Research others and see what the most-common interface is.
There was a problem hiding this comment.
Safe definitely does: getOwners() is a public view function on OwnerManager, which Safe inherits.
Just tested with foundry just now to double check but this same 'trick' worked with the Root Key Holders msig-of-msigs which mixed raw and Safe msigs.
But as for others I don't know - what others do we need to support?
There was a problem hiding this comment.
Sounds like we still only support safes.
Here's a list https://www.alchemy.com/dapps/list-of/multisig-wallets-on-ethereum
There was a problem hiding this comment.
OK cool, so we're OK then I think.
If we were still worried and want to be as broad as possible then we could change it to only check address.code.length > 0 like you suggested in the other thread. This is a governance opinion at this point I think.
5 participants
In keeping with the FIP text we only check that this is a multisig with more than 1 signer, no internal details are checked or enforced.
Note I think it's legit to make it >1 signer since otherwise it's too easy to lose control of this important key.